Skip to content

disco, pack: auction scheduler strategy - #12619

Open
topointon-jump wants to merge 13 commits into
mainfrom
auction-scheduler-2
Open

topointon-jump wants to merge 13 commits into
mainfrom
auction-scheduler-2

Conversation

@topointon-jump

@topointon-jump topointon-jump commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Credit to Hervé Guillemet (Puffin Validator)!

@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
 ┌─ ⚡ PERF · eae4438 vs main@c33077d ─────────────────────────────────
 │ SUITE                               BASELINE          NEW         Δ
 │ replay tps, mainnet               13,822 tps   13,817 tps  ·  -0.04%
 │ bench tps, localnet              661,257 tps  667,768 tps  ·  +0.98%
 │ snapshot load, testnet                4.94 s       4.93 s  ·  -0.17%
 │ snapshot load, mainnet               67.41 s      67.96 s  ·  +0.82%
 │ epoch boundary, mainnet             155.3 ms     155.6 ms  ·  +0.17%
!│ mem total, mainnet                143.84 GiB   143.84 GiB  ▲   0.00%
!│ mem total, testnet                 81.16 GiB    81.16 GiB  ▲   0.00%
 │ clean compile, firedancer             3.18 s       3.19 s  ·  +0.24%
 │ binary size, firedancer             58.64 MB     58.66 MB  ·  +0.03%
 ├─────────────────────────────────────────────────────────────────────
@@ 0 REGRESSIONS · 2 WARNINGS · 0 IMPROVED · 7 NOISE @@
 └─────────────────────────────────────────────────────────────────────
history · 10 pushes
 ┌─ HISTORY · Δ vs main, per push, newest first ───────────────────────────────────────────
 │ HEAD         TPS    BENCH   SNAP·T   SNAP·M    EPOCH    MEM·M    MEM·T  COMPILE   BINARY
!│ eae4438   -0.04%   +0.98%   -0.17%   +0.82%   +0.17%    0.00%    0.00%   +0.24%   +0.03%
!│ 0b611e5   +0.20%   -0.17%   -0.45%   +1.01%   -0.04%    0.00%    0.00%   -0.54%   +0.03%
!│ a86840a   +0.78%   +0.14%   -0.43%   +0.98%   +0.86%    0.00%    0.00%   -0.16%   +0.03%
!│ 1982073   +0.12%   +0.31%   +0.01%   -0.14%   -0.25%    0.00%    0.00%   -0.31%   +0.03%
!│ 726d1c9   -1.08%   -0.05%    0.00%   -0.04%   +1.10%    0.00%    0.00%   +0.69%   +0.03%
!│ bc3613e   +0.52%   -0.43%   -0.19%   +0.02%   +0.95%    0.00%    0.00%   -0.13%   +0.04%
+│ 45b0eb9   -0.39%   +0.62%   -2.75%        …   -0.09%    0.00%    0.00%   -0.88%   +0.03%
+│ cdbbb8a   -0.06%   -0.31%   -0.40%        …   -2.24%    0.00%    0.00%   -0.65%   +0.03%
!│ e7b2438   -0.07%   -0.22%   -0.03%        …   -1.25%    0.00%    0.00%   +0.20%   +0.03%
!│ 55ea7e8   -0.29%   -0.36%   +0.03%        …   +0.31%    0.00%    0.00%   -0.44%   +0.04%
 └─────────────────────────────────────────────────────────────────────────────────────────

@topointon-jump
topointon-jump marked this pull request as ready for review October 29, 2026 23:52
Copilot AI balanced review requested due to automatic review settings October 29, 2026 23:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The auction strategy is rejected by the bundled GUI’s unchanged schema, and the configuration documentation incorrectly claims votes bypass auction batching.

2 open issues
What changed in this PR

Adds an auction scheduling strategy to Firedancer’s pack tile, batching transactions while allowing bundles to remain independently schedulable.

Changes:

  • Adds auction timing, compute-unit pacing, and a holding queue for incoming transactions.
  • Exposes the strategy through configuration and GUI output.
  • Adds queue lifecycle tests and auction metrics with generated schemas and documentation.
File Description
src/​disco/​pack/​test_pack.c Tests auction holding, release, deletion, and expiry.
src/​disco/​pack/​fd_pack.h Declares auction lifecycle and query APIs.
src/​disco/​pack/​fd_pack.c Implements the holding queue and auction lifecycle.
src/​disco/​pack/​fd_pack_tile.c Integrates auction timing, pacing, and scheduling.
src/​disco/​metrics/​metrics.xml Defines auction metrics and end reasons.
src/​disco/​metrics/​generated/​fd_metrics_sample_gen.c Adds auction metric serialization.
src/​disco/​metrics/​generated/​fd_metrics_pack.h Adds generated metric offsets and definitions.
src/​disco/​metrics/​generated/​fd_metrics_enums.h Adds auction-related enum constants.
src/​disco/​metrics/​generated/​fd_metrics_all.c Registers auction metric metadata.
src/​disco/​gui/​fd_gui_printf.c Emits the auction strategy name.
src/​disco/​events/​schema/​metrics_pack.json Adds auction fields to the event schema.
src/​disco/​events/​schema/​events.proto Adds auction protobuf messages and fields.
src/​app/​shared/​fd_config.c Accepts the auction strategy setting.
src/​app/​firedancer/​config/​default.toml Documents the new strategy option.
book/​api/​metrics-generated.md Documents the generated auction metrics.

Review effort: Balanced

Comment thread src/disco/gui/fd_gui_printf.c Resolved
Comment thread src/app/firedancer/config/default.toml Resolved
@topointon-jump
topointon-jump removed this pull request from stack #12620 October 30, 2026 02:52
Copilot AI balanced review requested due to automatic review settings October 30, 2026 15:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The tests do not exercise bulk promotion with multiple entries, including a penalty treap, leaving transaction preservation and account-conflict enforcement unverified for the new traversal.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Medium severity Test bulk promotion from treaps with shared-account conflicts

src/​disco/​pack/​test_pack.c:1755

This only promotes one held transaction; the later three-transaction case deletes or expires every entry before ending the auction. Consequently, the new traversal that clears the source treap and reinserts its nodes is never exercised with multiple entries. Please add a bulk-promotion case, including enough shared-account transactions to enter a penalty treap, then verify the pack and drain it to check that no entries are lost and account conflicts remain enforced.

2 resolved since last review

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 30, 2026 16:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The auction-flush tests leave multi-entry traversal and conflict tracking untested; coverage should verify priority order and blocking of transactions sharing writable accounts until the preceding microblock completes.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Medium severity Test multi-entry auction flush traversal and conflict tracking

src/​disco/​pack/​fd_pack.c:3025

test_next_auction flushes only one transaction; its multi-entry case deletes or expires every entry before ending the auction. This leaves multi-entry traversal and conflict tracking during the flush untested. Add a case with multiple held transactions sharing writable accounts, then verify priority order and that conflicting work stays blocked until the preceding microblock completes.

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 30, 2026 16:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

An unpublished mismatch between the metric’s documented cutoff and its actual trigger requires human assessment of its impact on exported documentation and schema.

No open issues 🎉

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 30, 2026 16:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Timing-sensitive interactions among auction deadlines, CU pacing, bundle arrivals, and leader-slot transitions require representative runtime/load validation beyond the queue-focused unit test.

No open issues 🎉

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 30, 2026 17:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Human assessment is needed because auction-end tests leave multi-transaction transfers, shared-account allocation, and penalty handling untested, so transaction preservation and conflict enforcement remain unverified.

No open issues 🎉

Review effort: Balanced

Comment thread src/app/firedancer/config/default.toml Resolved
Copilot AI balanced review requested due to automatic review settings November 2, 2026 23:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The new test leaves multi-node auction flushing unverified when intrusive treap links are rewritten; a shared-account case must confirm reachability, exactly-once scheduling, and successful fd_pack_verify after transfer.

1 open issue
1 resolved since last review

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 3, 2026 16:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The tests lack a multi-transaction auction flush case covering shared-account penalty treaps, leaving transaction reachability and exactly-once scheduling unverified when reinsertion rewrites treap links.

1 open issue

Review effort: Balanced

Comment thread src/disco/pack/fd_pack_tile.c Resolved
Comment thread src/disco/pack/fd_pack_tile.c Resolved
Comment thread src/disco/pack/fd_pack_tile.c Resolved
@HGuillemet

Copy link
Copy Markdown
Contributor

You are using pacing to determine the number of execle tiles. What is the point ?
Pacing gives the number of execle needed to fill the slot if they are continuously running. Which is not the case
with batches.
I think pacing makes sense only for the last auction of a slot or window.

Copilot AI balanced review requested due to automatic review settings November 3, 2026 18:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The regression test does not cover ending an auction with multiple held transactions, leaving destructive traversal and pending/penalty treap reinsertion untested.

1 open issue

Review effort: Balanced

Comment thread src/disco/pack/fd_pack.c Resolved
Copilot AI balanced review requested due to automatic review settings November 3, 2026 20:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The new scheduler’s pacing, bundle timing, CU-rebate interactions, and slot transitions still need tile-level runtime validation beyond the added hold-queue tests.

1 open issue

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 3, 2026 20:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The timing-sensitive scheduler changes CU pacing, bundle handling, and slot transitions, but tests cover only queue mechanics, leaving scheduler interactions dependent on domain review and realistic runtime validation.

1 open issue

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 3, 2026 20:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The added test covers queue lifecycle only, leaving timing-sensitive auction pacing, bundle bursts, CU rebates, and slot-boundary behavior across multiple execle tiles in need of validator-level validation.

1 open issue

Review effort: Balanced

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants